Skip to content
This repository was archived by the owner on Jun 30, 2023. It is now read-only.

Handle properly Google's tokeninfo 503/backend error#141

Merged
tangiel merged 1 commit intocloudendpoints:masterfrom
AODocs:auth_errors
Apr 18, 2018
Merged

Handle properly Google's tokeninfo 503/backend error#141
tangiel merged 1 commit intocloudendpoints:masterfrom
AODocs:auth_errors

Conversation

@clementdenis
Copy link
Contributor

We are using Cloud Endpoints to serve a large scale API (millions of API requests per day).
We measure a low but significant amount of transient "503/backend error" when validating OAuth2 requests with GoogleAuth#getTokenInfoRemote, that translates into a "401/UnauthorizedException" for the API clients => this is not what one would expect for a transient exception.

This pull request introduce two main changes:

@codecov-io
Copy link

codecov-io commented Apr 16, 2018

Codecov Report

Merging #141 into master will increase coverage by 0.06%.
The diff coverage is 80.95%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #141      +/-   ##
============================================
+ Coverage     79.95%   80.02%   +0.06%     
- Complexity     1673     1677       +4     
============================================
  Files           156      156              
  Lines          5573     5591      +18     
  Branches        725      729       +4     
============================================
+ Hits           4456     4474      +18     
+ Misses          841      840       -1     
- Partials        276      277       +1
Impacted Files Coverage Δ Complexity Δ
...le/api/server/spi/auth/EndpointsAuthenticator.java 100% <ø> (ø) 6 <0> (ø) ⬇️
.../server/spi/auth/GoogleAppEngineAuthenticator.java 85.45% <ø> (ø) 16 <0> (ø) ⬇️
...api/server/spi/auth/GoogleOAuth2Authenticator.java 87.09% <ø> (ø) 10 <0> (ø) ⬇️
...ava/com/google/api/server/spi/auth/GoogleAuth.java 85.71% <80.95%> (+0.96%) 33 <5> (+3) ⬆️
...rver/spi/response/ServiceUnavailableException.java 14.28% <0%> (+14.28%) 1% <0%> (+1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36333f9...031de81. Read the comment docs.

Copy link
Contributor

@tangiel tangiel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM, just two minor comments. Thanks for doing this!

@VisibleForTesting
static void configureErrorHandling(HttpRequest request) {
request.setNumberOfRetries(1)
.setThrowExceptionOnExecuteError(false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be indented four spaces in.

.setIOExceptionHandler(new HttpIOExceptionHandler() {
@Override
public boolean handleIOException(HttpRequest request, boolean supportsRetry) {
return true; //consider all IOException as transient
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super minor nit but please add a space after the // here and below.

@clementdenis
Copy link
Contributor Author

Fixed formatting (and too long lines as well). As there are only format change, I amended my commit and force pushed.

I'm coding with IntelliJ, can you confirm I should use Google's style guide?

Copy link
Contributor

@tangiel tangiel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that should cover most of the formatting.

@tangiel tangiel merged commit 1b51f85 into cloudendpoints:master Apr 18, 2018
@clementdenis clementdenis deleted the auth_errors branch July 31, 2018 12:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants